Skip to content

Dencun header changes#7993

Merged
ajsutton merged 23 commits intodevelopfrom
dencun-header-change
Nov 8, 2023
Merged

Dencun header changes#7993
ajsutton merged 23 commits intodevelopfrom
dencun-header-change

Conversation

@EvanJRichard
Copy link
Contributor

@EvanJRichard EvanJRichard commented Nov 1, 2023 •
edited
Loading

Description

This PR seeks to cherry pick a subset of #7349. Specifically, we just want the rpc-header change in op-service/sources, the basic types in op-service/eth, and we also seek to add some action test like TestShapellaL1Fork in #5144.

Tests

Adds a new test TestDencunL1Fork in op-e2e/actions/dencun_fork_test.go, which just makes sure that the L2 can catch up to an L1 that supports Cancun.

Additional context

Thank you Roberto and Proto.

Metadata

Co-authored-by: protolambda proto@protolambda.com
Co-authored-by: Roberto Bayardo bayardo@alum.mit.edu

semgrep-app[bot]
semgrep-app bot reviewed Nov 1, 2023
View reviewed changes
semgrep-app[bot]
semgrep-app bot reviewed Nov 1, 2023
View reviewed changes
trianglesphere
trianglesphere reviewed Nov 6, 2023
View reviewed changes
@trianglesphere
Copy link
Contributor

trianglesphere commented Nov 6, 2023

This needs a merge in / rebase from develop to fix the bindings build. Not sure about the other tests yet though

@EvanJRichard
Copy link
Contributor Author

EvanJRichard commented Nov 6, 2023 •
edited
Loading

@trianglesphere git is telling me the PR is up to date with develop, but the bindings build is still broken - am I maybe mistaken that it's up to date, or something? EDIT Yes I was mistaken

@EvanJRichard EvanJRichard marked this pull request as ready for review November 6, 2023 20:11
@EvanJRichard EvanJRichard requested a review from a team as a code owner November 6, 2023 20:11
EvanJRichard and others added 19 commits November 6, 2023 15:14
@protolambda @roberto-bayardo
Pull in basic types from op-service/eth package.
556e500 
Co-authored-by: protolambda <proto@protolambda.com>
Co-authored-by: Roberto Bayardo <bayardo@alum.mit.edu>
@roberto-bayardo @protolambda
Pull in basic types from op-service/sources.
ba2ec85 
Co-authored-by: Roberto Bayardo <bayardo@alum.mit.edu>
Co-authored-by: protolambda <proto@protolambda.com>
@protolambda @roberto-bayardo
Pull in blob and blob_test from PR.
6b4cb22 
Co-authored-by: protolambda <proto@protolambda.com>
Co-authored-by: Roberto Bayardo <bayardo@alum.mit.edu>
@protolambda @roberto-bayardo
Revert "Pull in basic types from op-service/eth package."
78716c9 
This reverts commit 7eccd9f.

Co-authored-by: protolambda <proto@protolambda.com>
Co-authored-by: Roberto Bayardo <bayardo@alum.mit.edu>
Add placeholder for dencun fork test.
c174cb5
Add filler scaffolding for dencun fork test: literally the old shapel…
149a6bc 
…la test.
Start to tweak things in dencun_fork_test: mainly replacing "shanghai…
4806862 
…" with "Cancun"
More minor test tweaks: change require statement's message, get head'…
adcdf91 
…s time differently, add a couple breadcrumb comments.
Fix usage of IsCancun.
118ce7f
Tweak test message: really we're just testing isCancun, so acknowledg…
d0a7922 
…e that.
Use types.Withdrawals in rpcBlock, not eth.Withdrawals.
5c584ca
Flag the failing line, it's non-obvious in CI output.
d57f75f
Add how test is failing just for onlookers.
bc2b4de
Add issue links per semgrep request.
79932ba
WIP stash for discussion.
1131944
Commit missed hunk editing a comment in dencun_fork_test.
8389e8c
Remove useless call to ParentBeaconRoot.
acf1c64
Remove some commented-out cruft.
65916c5
Set necessary header fields if config is cancun.
2c57e7d
@protolambda @roberto-bayardo
Preserve co-authorship.
78eeb2a 
Co-authored-by: protolambda <proto@protolambda.com>
Co-authored-by: Roberto Bayardo <bayardo@alum.mit.edu>
ajsutton
ajsutton requested changes Nov 6, 2023
View reviewed changes
Copy link
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We seem to have lost a couple of parts of the commit 73c7512 but we'll want to keep them. I'm somewhat surprised that there aren't failures in CI though as that commit had plenty of tests...

Ooo, the test is failing on the bad block hash, not on the withdrawals being invalid. That's a bug in the test. I'll put up a separate PR to fix the tests in a bit.

Still looking through the actual blob verification side too but figured I'd post this now.

ajsutton
ajsutton reviewed Nov 6, 2023
View reviewed changes
@ajsutton ajsutton mentioned this pull request Nov 6, 2023
Merged
@semgrep-app
Copy link
Contributor

semgrep-app bot commented Nov 7, 2023

Semgrep found 1 nil-check-after-call finding:

Potential p2pmetrics nil dereference when NewBandwidthCounter is called

Ignore this finding from nil-check-after-call.

@EvanJRichard
Copy link
Contributor Author

EvanJRichard commented Nov 7, 2023

Ignoring semgrep nil-after-call finding.

ajsutton
ajsutton approved these changes Nov 7, 2023
View reviewed changes
Copy link
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@ajsutton ajsutton added this pull request to the merge queue Nov 8, 2023
Merged via the queue into develop with commit 5b67802 Nov 8, 2023
@ajsutton ajsutton deleted the dencun-header-change branch November 8, 2023 03:22
@EvanJRichard EvanJRichard mentioned this pull request Nov 8, 2023
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tynes tynes tynes left review comments

@ajsutton ajsutton ajsutton approved these changes

@trianglesphere trianglesphere Awaiting requested review from trianglesphere

+2 more reviewers

@protolambda protolambda protolambda left review comments

@semgrep-app semgrep-app[bot] semgrep-app[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@sebastianst sebastianst

@EvanJRichard EvanJRichard

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@EvanJRichard @trianglesphere @ajsutton @tynes @protolambda @sebastianst